Skip to content

C++: Model more stringstream features#4253

Merged
jbj merged 8 commits intogithub:mainfrom
geoffw0:stringstream2
Sep 15, 2020
Merged

C++: Model more stringstream features#4253
jbj merged 8 commits intogithub:mainfrom
geoffw0:stringstream2

Conversation

@geoffw0
Copy link
Copy Markdown
Contributor

@geoffw0 geoffw0 commented Sep 11, 2020

Model std::stringstream constructor, str, put and write.

More to follow, but there's a small change in DataFlowUtil.qll I'd like to get through review now.

@geoffw0 geoffw0 added the C++ label Sep 11, 2020
@geoffw0 geoffw0 requested a review from a team as a code owner September 11, 2020 10:20
@jbj jbj self-requested a review September 11, 2020 11:56
Copy link
Copy Markdown
Contributor

@jbj jbj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise LGTM

Comment thread cpp/ql/src/semmle/code/cpp/models/implementations/StdString.qll
Comment thread cpp/ql/src/semmle/code/cpp/models/implementations/StdString.qll
// flow from qualifier to return value
input.isQualifierAddress() and
input.isQualifierObject() and
output.isReturnValue()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't agree with this change. The qualifier object is a stringstream, but the return value is a stringstream &.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think you're right. These methods return a reference to their qualifier (*this), and I think I got confused by the reference (I think I'm supposed to always think of references as pointers for data flow).

That said we lose some flows if I change it back to isQualifierAddress. I'm looking into this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've fixed the lost flows with a change to DataFlowUtil.qll. Please check that though - I'm not entirely sure how the dataflow library is supposed to deal with references.

Comment on lines +643 to +647
inModel.isQualifierObject() and
fromExpr = call.getQualifier()
or
inModel.isQualifierAddress() and
fromExpr = call.getQualifier()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to accept this conflation between pointers and objects in AST data flow as long as there's no evidence that it causes bad results from real-world queries.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the call is through . then getQualifier() is the object, whereas if the call is through -> then getQualifier() is the address. The getQualifier() of a call to operator<< can be either a class or reference. I think the conflation was already there (and this is an area where our QL libraries aren't super helpful at the moment).

@jbj jbj merged commit 25412da into github:main Sep 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants